feat: add payment module params #893

rahulghangas · 2022-10-20T12:55:54Z

Adds minSquareSize and maxSquareSize as params to the payment module.
Defines relevant stores and queries

Relevant changes in
proto/payment/*
x/payment/keeper*
x/payment/types/*

closes Add the Max and MinSquareSize as a parameter to the payment module #183

Note: The constants that currently define min/max square size have not been deprecated, will do so in another PR

rach-id

Good start 👍
Did a first pass and left some blocking comments.
Also, doesn't it make sense also to make the corresponding genesis changes?
I once set some parameters for the QGB, and I updated the following when init and export:
https://github.com/celestiaorg/celestia-app/blob/main/x/qgb/genesis.go

proto/payment/params.proto

proto/payment/query.proto

x/payment/keeper/params.go

x/payment/types/params.go

rootulp

golangci-lint is failing here (likely fixable via make fmt)
proto-gen is failing here (likely fixable via make proto-gen)

proto/payment/params.proto

x/payment/keeper/params.go

x/payment/types/params.go

rootulp

Thanks for addressing feedback so quickly!

I'm observing the following panic when attempting to test the CLI query command.

$ celestia-appd query payment params
Error: rpc error: code = Unknown desc = parameter MinSquareSize not registered: panic

To reproduce this error, I checked out this branch locally and installed the binary (i.e make install)

rootulp · 2022-10-20T17:43:05Z

proto/payment/params.proto

+message Params {
+  option (gogoproto.goproto_stringer) = false;
+
+  uint32 min_square_size = 1 [(gogoproto.moretags) = "yaml:\"min_square_size\""];


[question] for my learning, why is the "yaml:\"min_square_size\"" necessary? In the future, do we plan on reading this value from a YAML file? It looks like removing it has the following effect on params.pb.go:

type Params struct { - MinSquareSize uint32 `protobuf:"varint,1,opt,name=min_square_size,json=minSquareSize,proto3" json:"min_square_size,omitempty" yaml:"min_square_size"` - MaxSquareSize uint32 `protobuf:"varint,2,opt,name=max_square_size,json=maxSquareSize,proto3" json:"max_square_size,omitempty" yaml:"max_square_size"` + MinSquareSize uint32 `protobuf:"varint,1,opt,name=min_square_size,json=minSquareSize,proto3" json:"min_square_size,omitempty"` + MaxSquareSize uint32 `protobuf:"varint,2,opt,name=max_square_size,json=maxSquareSize,proto3" json:"max_square_size,omitempty"` }

It's standard practice from what I've seen. Also, it makes it very easy to define genesis parameters through a config.yml file

For eg. https://github.com/osmosis-labs/osmosis/blob/main/proto/osmosis/lockup/params.proto

I can't find a config.yml in https://github.com/osmosis-labs/osmosis so I'm not sure what you mean. Can you please elaborate or include a link to the file?

I still don't understand the benefit to using yaml tags but this isn't blocking so feel free to resolve the conversation @rahulghangas if there aren't updates here

rahulghangas · 2022-10-20T18:08:53Z

I'm observing the following panic when attempting to test the CLI query command.
$ celestia-appd query payment params
Error: rpc error: code = Unknown desc = parameter MinSquareSize not registered: panic
To reproduce this error, I checked out this branch locally and installed the binary (i.e make install)

Whoops, fixing this

evan-forbes

Nice!! these will be really useful in the future.

The constants that currently define min/max square size have not been deprecated, will do so in another PR

IMO, we should actually keep these, but just change them to be the defaults. This way other projects import really similar defaults without booting up an application.

x/payment/types/params.go

x/payment/client/cli/query.go

proto/payment/params.proto

x/payment/keeper/grpc_query_params.go

codecov-commenter · 2022-10-20T18:39:50Z

Codecov Report

Merging #893 (cab4172) into main (22d366f) will decrease coverage by 2.24%.
The diff coverage is 8.36%.

@@            Coverage Diff             @@
##             main     #893      +/-   ##
==========================================
- Coverage   29.74%   27.49%   -2.25%     
==========================================
  Files          72       81       +9     
  Lines        8199     9091     +892     
==========================================
+ Hits         2439     2500      +61     
- Misses       5531     6355     +824     
- Partials      229      236       +7

Impacted Files	Coverage Δ
app/app.go	`6.44% <0.00%> (-0.06%)`	⬇️
app/malleate_txs.go	`71.87% <ø> (ø)`
app/parse_txs.go	`64.00% <ø> (ø)`
app/process_proposal.go	`0.00% <ø> (ø)`
app/test_util.go	`73.77% <ø> (ø)`
x/blob/handler.go	`0.00% <ø> (ø)`
x/blob/keeper/msg_server.go	`0.00% <ø> (ø)`
x/blob/module.go	`3.70% <ø> (ø)`
x/blob/payfordata.go	`0.00% <ø> (ø)`
x/blob/types/builder_options.go	`41.26% <ø> (ø)`
... and 27 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

rootulp

In one terminal, I ran

make install
celestia-appd start

and in a separate terminal, I'm observing a new error now:

$ celestia-appd query payment params
Error: rpc error: code = Unknown desc = UnmarshalJSON cannot decode empty bytes: panic

@rahulghangas are you able to reproduce this?

testutil/nullify/nullify.go

x/payment/genesis_test.go

x/payment/keeper/keeper.go

x/payment/keeper/params.go

x/payment/types/params.go

evan-forbes · 2022-10-25T14:36:11Z

@rootulp

In one terminal, I ran
make install
celestia-appd start
and in a separate terminal, I'm observing a new error now:

did you initialize the chain from scratch? the only way to set genesis values after upgrading binaries is to include it in an upgrade handler or perhaps vote on it. In a normal situation, it will be included in the genesis.

rootulp · 2022-10-25T15:03:40Z

did you initialize the chain from scratch?

Nope. Thanks for identifying my mistake. I just did and this command now works as expected

$ celestia-appd query payment params
params:
  max_square_size: 128
  min_square_size: 1

rootulp · 2022-10-26T03:51:13Z

[optional][can be a follow-up PR] we likely want to update https://github.com/celestiaorg/celestia-app/blob/main/x/payment/spec/docs.md#parameters

evan-forbes

LGTM utACK

x/payment/types/params.go

testutil/nullify/nullify.go

x/payment/genesis_test.go

rach-id

LGTM, just some final comments

x/blob/types/params.go

testutil/keeper/payment.go

x/blob/types/params.go

evan-forbes

lets mergeeeeeeee

Adds `gasPerMsgByte` as params to the payment module. Defines relevant stores and queries Relevant changes in `proto/payment/*` `x/payment/keeper*` `x/payment/types/*` - [x] finished 1st part of #949 Note: To be merged after #893 and constants for default values can be defined in a cumulative PR

## TODO - [x] Split out changes that are only relevant post ADR 008 implementation and target feature branch - [x] Document gas cost for a message - [x] [Potentially] remove outdated code blocks from README.md ## Future PRs - [ ] Params section needs to be updated after params are added to payment module celestiaorg#893 - [ ] [Potentially] add client section (motivated by https://github.com/cosmos/cosmos-sdk/tree/main/x/auth#client)

Adds `minSquareSize` and `maxSquareSize` as params to the payment module. Defines relevant stores and queries Relevant changes in `proto/payment/*` `x/payment/keeper*` `x/payment/types/*` - [x] closes celestiaorg#183 Note: The constants that currently define min/max square size have not been deprecated, will do so in another PR

Adds `gasPerMsgByte` as params to the payment module. Defines relevant stores and queries Relevant changes in `proto/payment/*` `x/payment/keeper*` `x/payment/types/*` - [x] finished 1st part of celestiaorg#949 Note: To be merged after celestiaorg#893 and constants for default values can be defined in a cumulative PR

## TODO - [x] Split out changes that are only relevant post ADR 008 implementation and target feature branch - [x] Document gas cost for a message - [x] [Potentially] remove outdated code blocks from README.md ## Future PRs - [ ] Params section needs to be updated after params are added to payment module celestiaorg#893 - [ ] [Potentially] add client section (motivated by https://github.com/cosmos/cosmos-sdk/tree/main/x/auth#client)

Adds `minSquareSize` and `maxSquareSize` as params to the payment module. Defines relevant stores and queries Relevant changes in `proto/payment/*` `x/payment/keeper*` `x/payment/types/*` - [x] closes celestiaorg#183 Note: The constants that currently define min/max square size have not been deprecated, will do so in another PR

Adds `gasPerMsgByte` as params to the payment module. Defines relevant stores and queries Relevant changes in `proto/payment/*` `x/payment/keeper*` `x/payment/types/*` - [x] finished 1st part of celestiaorg#949 Note: To be merged after celestiaorg#893 and constants for default values can be defined in a cumulative PR

Resolves [this feedback](#893 (comment)). Inspired by https://github.com/cosmos/cosmos-sdk/tree/main/x/auth#parameters

## TODO - [x] Split out changes that are only relevant post ADR 008 implementation and target feature branch - [x] Document gas cost for a message - [x] [Potentially] remove outdated code blocks from README.md ## Future PRs - [ ] Params section needs to be updated after params are added to payment module celestiaorg/celestia-app#893 - [ ] [Potentially] add client section (motivated by https://github.com/cosmos/cosmos-sdk/tree/main/x/auth#client)

Resolves [this feedback](celestiaorg/celestia-app#893 (comment)). Inspired by https://github.com/cosmos/cosmos-sdk/tree/main/x/auth#parameters

rahulghangas requested a review from evan-forbes as a code owner October 20, 2022 12:55

rahulghangas requested review from rootulp and rach-id October 20, 2022 12:56

rach-id reviewed Oct 20, 2022

View reviewed changes

rach-id assigned rahulghangas Oct 20, 2022

rach-id added enhancement New feature or request C: Celestia app warn:api breaking item will be break an API and require a major bump labels Oct 20, 2022

rahulghangas mentioned this pull request Oct 20, 2022

Deprecate consts for MaxSquareSize and MinSquareSize #895

Closed

rootulp requested changes Oct 20, 2022

View reviewed changes

rahulghangas requested review from rootulp and rach-id October 20, 2022 17:50

rootulp reviewed Oct 20, 2022

View reviewed changes

evan-forbes reviewed Oct 20, 2022

View reviewed changes

x/payment/types/params.go Outdated Show resolved Hide resolved

x/payment/client/cli/query.go Outdated Show resolved Hide resolved

proto/payment/params.proto Outdated Show resolved Hide resolved

x/payment/keeper/grpc_query_params.go Outdated Show resolved Hide resolved

rahulghangas requested review from evan-forbes and rootulp October 25, 2022 08:33

rootulp reviewed Oct 25, 2022

View reviewed changes

rahulghangas force-pushed the feat/payment-module-params branch from fb7c394 to 8f00dc9 Compare October 31, 2022 15:34

evan-forbes previously approved these changes Oct 31, 2022

View reviewed changes

rach-id requested changes Oct 31, 2022

View reviewed changes

x/payment/types/params.go Outdated Show resolved Hide resolved

testutil/nullify/nullify.go Outdated Show resolved Hide resolved

rach-id requested changes Nov 1, 2022

View reviewed changes

x/payment/genesis_test.go Outdated Show resolved Hide resolved

This was referenced Nov 3, 2022

Extract GasPerMsgByte as a parameter to payment module #949

Closed

Update docs for payment module #948

Merged

rahulghangas dismissed evan-forbes’s stale review via d3a59ba November 3, 2022 08:26

rahulghangas requested a review from rootulp November 3, 2022 08:31

rahulghangas dismissed stale reviews from rootulp and rach-id via 713ca62 November 10, 2022 06:42

rahulghangas added 3 commits November 10, 2022 12:13

chore: grammar

807bc7d

chore: grammar

1bb4b1e

chore: rename payment to blob

cab4172

rach-id reviewed Nov 10, 2022

View reviewed changes

x/blob/types/params.go Show resolved Hide resolved

testutil/keeper/payment.go Outdated Show resolved Hide resolved

rahulghangas added 3 commits November 10, 2022 15:52

chore: rename payment to blob

d8617cd

chore: add order validation for min/max square size

7b93b93

chore: min and max square size can be same

837bdaa

rahulghangas requested review from rach-id and rootulp November 10, 2022 10:25

rach-id reviewed Nov 10, 2022

View reviewed changes

x/blob/types/params.go Outdated Show resolved Hide resolved

fix: typo

17669da

evan-forbes approved these changes Nov 10, 2022

View reviewed changes

rahulghangas merged commit a5272de into celestiaorg:main Nov 10, 2022

rahulghangas deleted the feat/payment-module-params branch November 10, 2022 14:46

rahulghangas mentioned this pull request Nov 17, 2022

Replace all default value usage with values from KVStore #1032

Closed

This was referenced Nov 17, 2022

chore: move default const values to appconsts module #1031

Merged

docs: payment module params #1058

Merged

rootulp added a commit that referenced this pull request Nov 23, 2022

docs: payment module params (#1058)

9884e5d

Resolves [this feedback](#893 (comment)). Inspired by https://github.com/cosmos/cosmos-sdk/tree/main/x/auth#parameters

evan-forbes added this to the Incentivized Testnet Stretch milestone Jan 3, 2023

0xchainlover pushed a commit to celestia-org/celestia-app that referenced this pull request Aug 1, 2024

docs: payment module params (#1058)

25bcb0f

Resolves [this feedback](celestiaorg/celestia-app#893 (comment)). Inspired by https://github.com/cosmos/cosmos-sdk/tree/main/x/auth#parameters

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add payment module params #893

feat: add payment module params #893

rahulghangas commented Oct 20, 2022 •

edited

Loading

rach-id left a comment

rootulp left a comment

rootulp left a comment

rootulp Oct 20, 2022

rahulghangas Oct 20, 2022

rootulp Oct 20, 2022

rootulp Nov 3, 2022

rahulghangas commented Oct 20, 2022

evan-forbes left a comment •

edited

Loading

codecov-commenter commented Oct 20, 2022 •

edited

Loading

rootulp left a comment

evan-forbes commented Oct 25, 2022

rootulp commented Oct 25, 2022

rootulp commented Oct 26, 2022

evan-forbes left a comment

rach-id left a comment

evan-forbes left a comment

feat: add payment module params #893

feat: add payment module params #893

Conversation

rahulghangas commented Oct 20, 2022 • edited Loading

rach-id left a comment

Choose a reason for hiding this comment

rootulp left a comment

Choose a reason for hiding this comment

rootulp left a comment

Choose a reason for hiding this comment

rootulp Oct 20, 2022

Choose a reason for hiding this comment

rahulghangas Oct 20, 2022

Choose a reason for hiding this comment

rootulp Oct 20, 2022

Choose a reason for hiding this comment

rootulp Nov 3, 2022

Choose a reason for hiding this comment

rahulghangas commented Oct 20, 2022

evan-forbes left a comment • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Oct 20, 2022 • edited Loading

Codecov Report

rootulp left a comment

Choose a reason for hiding this comment

evan-forbes commented Oct 25, 2022

rootulp commented Oct 25, 2022

rootulp commented Oct 26, 2022

evan-forbes left a comment

Choose a reason for hiding this comment

rach-id left a comment

Choose a reason for hiding this comment

evan-forbes left a comment

Choose a reason for hiding this comment

rahulghangas commented Oct 20, 2022 •

edited

Loading

evan-forbes left a comment •

edited

Loading

codecov-commenter commented Oct 20, 2022 •

edited

Loading